Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add framework for key exchange schemes and Diffie-Hellman #38374

Merged
merged 32 commits into from
Aug 3, 2024

Conversation

vincentmacri
Copy link
Contributor

@vincentmacri vincentmacri commented Jul 15, 2024

Motivated by (but does not yet close) #37305. Closes #11568.

This PR adds a basic framework to add public key exchange schemes to Sage, and includes an implementation of the Diffie-Hellman primarily as an example of this new framework.

Open to suggestions to improve the structure of the code. This code was based on the existing code for public-key encryption in Sage.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

None

@vincentmacri
Copy link
Contributor Author

@grhkm21 Thoughts on this?

@vincentmacri vincentmacri changed the title First work to add public key exchange schemes Add framework for public key exchange schemes and Diffie-Hellman Jul 15, 2024

class KeyExchangeScheme:

@experimental(37305)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is marked as experimental because if this gets merged I'd like to add more cryptographic schemes to Sage in the near(ish) future as I have spare time. I don't want to be tied down yet to any particular way of structuring the code.


class DiffieHellman(KeyExchangeScheme):

@experimental(37305)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on KeyExchangeScheme on why this is marked experimental.

Copy link

github-actions bot commented Jul 16, 2024

Documentation preview for this PR (built with commit fc1a4dc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@vincentmacri vincentmacri marked this pull request as ready for review July 16, 2024 21:29
Copy link
Contributor

@JosePisco JosePisco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me overall. Just being picky with style issues. We should check if docstring EXAMPLES and TESTS should be formatted and if so, how to do it properly.

Nice contribution, it's good to see old issues being solved ^^'

src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/key_exchange.py Outdated Show resolved Hide resolved
Co-authored-by: grnx <47418794+JosePisco@users.noreply.github.com>
@grhkm21
Copy link
Contributor

grhkm21 commented Jul 17, 2024

I can review later. I just got back from vacation.

@vincentmacri vincentmacri changed the title Add framework for public key exchange schemes and Diffie-Hellman Add framework for key exchange schemes and Diffie-Hellman Jul 18, 2024
@vincentmacri
Copy link
Contributor Author

I can't find anything specific. Since the docstring EXAMPLES go into the documentation, I think it's most important that they're readable there.
I'm guessing you raised this because some of the examples are over 80 characters in the source code. I think most instances of this are because of the indenting before an example is already 12 characters. I don't think splitting from sage.crypto.key_exchange.diffie_hellman import DiffieHellman into two lines in the examples will help readability. I'm not sure there's much to be done for the tests where the return values are over 80 characters. I assume this is why the linter doesn't enforce the 80 character rule.

Yes this would make sense and formatting examples would probably make them less readable for the next person.

The rest of the code looks really neat 👍

Thanks for the review!

The last commit was just changing "public key exchange" to "key exchange" which I realized is the more standard terminology.

@vincentmacri
Copy link
Contributor Author

@grhkm21 Sorry for the ping, but could I get a review on this?

Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments. I have a few more comments (I think) but I have to test your code first, and my Sage is building now.

from sage.misc.lazy_import import lazy_import

lazy_import('sage.crypto.key_exchange.diffie_hellman', 'DiffieHellman')
del lazy_import
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't think of this before, but seems like a good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied from the crypto/public_key folder for this.

I'm now thinking I may have done this wrong (and maybe public_key did too but I think that's out of scope for this PR, public_key needs some refactoring I think). I'm going to do some more testing of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was doing it wrong. After the commit I just pushed, DiffieHellman can now be accessed without the user needing to explicitly import it.

This does raise the question of how we want the schemes to be accessed by the user. Most people probably won't use this feature (although I guess that applies to most things in Sage) so maybe it makes sense to access it as something like crypto.DiffieHellman, to avoid polluting the auto-suggest feature in some interfaces, but several of the existing symmetric ciphers are just accessed directly so I've done the same for consistency.

Happy to change it to something like crypto.DiffieHellman or key_exchange.DiffieHellman if that's preferred though.

Copy link
Contributor Author

@vincentmacri vincentmacri Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I have it so DiffieHellman is accessible without import, but KeyExchangeScheme requires an import. I don't think an abstract base class needs to be exposed to users without them explicitly importing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my PR of a similar nature, I created a catalog file, see here. (Suggested by Travis, another Sage contributor)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having key_exchange in global scope (I think?) is a bit questionable. Do you think it's a good idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's less questionable than SubstitutionCryptosystem being global scope (which it currently is).

I don't think it's too different than how distributions is global scope in the PR you linked. The alternative (which maybe is better) would be to access the key exchange schemes under crypto.key_exchange, so you'd do crypto.key_exchange.DiffieHellman. We could then in the future move the other cryptography code to crypto.[something] as well and take things like SubstitutionCryptosystem out of global scope.

After writing that I've convinced myself that it should be changed to crypto.key_exchange.

And the reason I didn't consider putting it under crypto directly (so crypto.DiffieHellman) is some schemes (like RSA) have multiple uses. RSA can be used for public key encryption, key exchange, and cryptographic signing. I think having something like crypto.pke.RSA, crypto.key_exchange.RSA, and crypto.signing.RSA would be better than crypto.RSAEncryption, crypto.RSAKeyExchange, and crypto.RSASign. Especially because for discoverability, a user is more likely to want to know "what key exchange schemes are in Sage" than "what RSA variants are in Sage". Thinking about the future and how many cryptography schemes (public key encryption, key exchange, signing, symmetric ciphers, etc.) could be implemented in Sage, putting it all under crypto would be too much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think crypto.key_exchange.DiffieHellman looks good (definitely not crypto.DiffieHellman), but again I don't have a strong opinion.

After writing that I've convinced myself that it should be changed to crypto.key_exchange.

Lol, it happens :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I realized that making it accessible under crypto.key_exchange would involve some major restructuring to the crypto module and I think that would be out of scope for this PR. Unless I'm forgetting some syntax, I don't think Python import statements let you do from a.b.c import x as y.z. Basically above I was advocating to change from sage.crypto.all import * to import sage.crypto.all as crypto in the all.py file in src/sage which currently only stats does (but stats still imports everything to global scope anyway).

For now I think it's fine to leave it as key_exchange in global scope. It's at least better than what we have for other stuff in the crypto module. But if you have other suggestions I'm open.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay if it's too much irrelevant stuff for this PR then it's okay to leave it (and optionally make a new issue that hopefully someone gets to.)

src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
src/sage/crypto/key_exchange/diffie_hellman.py Outdated Show resolved Hide resolved
@vincentmacri vincentmacri requested a review from grhkm21 July 30, 2024 20:16
@grhkm21
Copy link
Contributor

grhkm21 commented Jul 30, 2024

I'll look through it again once but I think it's okay. I can approve when you reply to the {crypto.}key_exchange.DiffieHellman scope thing

Copy link
Contributor

@grhkm21 grhkm21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Looking forward to implementing some crypto protocols myself too! Get people away from Magma x)

@grhkm21
Copy link
Contributor

grhkm21 commented Jul 31, 2024

conda 3.11 CI is failing but clearly the errors are irrelevant.

@vincentmacri
Copy link
Contributor Author

Thanks for the review! By the way, do you know what needs to happen for the PR status to change from "needs review" to "positive review"?

Looking forward to implementing some crypto protocols myself too!

To avoid duplicated work, I'll say that I'll probably be doing ECDH next, then CSIDH.

@grhkm21
Copy link
Contributor

grhkm21 commented Jul 31, 2024

@vincentmacri As long as there is positive review (and not disputed e.g. one positive one negative) then you or the reviewers can change it to positive review. I forgot to, thanks for the reminder.

Regarding the crypto, I have some CSIDH code lying around too if you want them.

@vincentmacri
Copy link
Contributor Author

@vincentmacri As long as there is positive review (and not disputed e.g. one positive one negative) then you or the reviewers can change it to positive review. I forgot to, thanks for the reminder.

Thanks! I don't think I have the permission to set labels so I can't do it myself.

Regarding the crypto, I have some CSIDH code lying around too if you want them.

Sure!

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 2, 2024
…ellman

    
Motivated by (but does not yet close) sagemath#37305. Closes sagemath#11568.

This PR adds a basic framework to add public key exchange schemes to
Sage, and includes an implementation of the Diffie-Hellman primarily as
an example of this new framework.

Open to suggestions to improve the structure of the code. This code was
based on the existing code for public-key encryption in Sage.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

None
    
URL: sagemath#38374
Reported by: Vincent Macri
Reviewer(s): grhkm21, grnx, Vincent Macri
@vbraun vbraun merged commit 316f656 into sagemath:develop Aug 3, 2024
25 of 26 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 3, 2024
@vincentmacri vincentmacri deleted the pke branch August 6, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diffie-Hellman key exchange
5 participants